Migrate to use redux-remember instead of redux-persist by zehata · Pull Request #4379 · nusmodifications/nusmods

zehata · 2026-03-31T07:15:47Z

This was squashed from a number of commits from https://github.com/zehata/nusmods/tree/redux-remember-test

Context

redux-persist is last committed to in 2021.
Objectively speaking, this is adding technical debt dating back to 2020: 4b484cf.

nusmods/website/src/reducers/timetables.ts

Lines 38 to 41 in 9e0e84f

    
           // FIXME: Remove the next line when _persist is optional again. 
        
           // Cause: https://github.com/rt2zz/redux-persist/pull/919 
        
           // Issue: https://github.com/rt2zz/redux-persist/pull/1170 
        
           // eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain

nusmods/website/src/reducers/timetables.ts

Lines 47 to 50 in 9e0e84f

    
           // FIXME: Remove the next line when _persist is optional again. 
        
           // Cause: https://github.com/rt2zz/redux-persist/pull/919 
        
           // Issue: https://github.com/rt2zz/redux-persist/pull/1170 
        
           // eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain

nusmods/website/src/reducers/moduleBank.ts

Lines 137 to 140 in 9e0e84f

    
           // FIXME: Remove the next line when _persist is optional again. 
        
           // Cause: https://github.com/rt2zz/redux-persist/pull/919 
        
           // Issue: https://github.com/rt2zz/redux-persist/pull/1170 
        
           // eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain

With the push to move towards agentic involvement, #4314, there may be a need to migrate over to a more recent solution.

To be quite honest, I didn't spend too long looking for an alternative, and redux-remember is the only one I have found. You should try looking for alternatives to redux-persist and redux-remember to decide if this is a prudent replacement.

Implementation

WIP

Note that I am not removing the redux-persist key-values in localStorage, just in case something goes wrong and we need to rollback. It will be trivial to remove in a future PR, perhaps along with the migration code.

Other Information

Obviously, with such a fundamental change to how we are storing user data, I think it is only prudent if we wait until the end of the semester.

vercel · 2026-03-31T07:15:53Z

@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

codecov · 2026-03-31T07:19:36Z

Codecov Report

❌ Patch coverage is 56.00000% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.24%. Comparing base (988c6fd) to head (e7fed0e).
⚠️ Report is 236 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/reducers/timetables.ts	42.85%	8 Missing ⚠️
website/src/storage/RehydrateGate.tsx	0.00%	6 Missing ⚠️
website/src/bootstrapping/configure-store.ts	55.55%	4 Missing ⚠️
website/src/reducers/reduxRemember.ts	42.85%	4 Missing ⚠️
website/src/storage/index.ts	72.72%	3 Missing ⚠️
...e/src/bootstrapping/migrate-persist-to-remember.ts	71.42%	2 Missing ⚠️
website/src/entry/main.tsx	0.00%	2 Missing ⚠️
website/src/entry/App.tsx	0.00%	1 Missing ⚠️
website/src/entry/export/main.tsx	0.00%	1 Missing ⚠️
website/src/middlewares/state-sync-middleware.ts	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4379      +/-   ##
==========================================
+ Coverage   54.52%   56.24%   +1.72%     
==========================================
  Files         274      320      +46     
  Lines        6076     6980     +904     
  Branches     1455     1688     +233     
==========================================
+ Hits         3313     3926     +613     
- Misses       2763     3054     +291

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- This was squashed from a number of commits from https://github.com/zehata/nusmods/tree/redux-remember-test

- Previous state during rehydration is not the default state, but rather the rehydrated state itself

vercel · 2026-05-24T08:06:36Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
nusmods-export	Ignored	Preview	May 24, 2026 8:06am
nusmods-website	Ignored	Preview	May 24, 2026 8:06am

ravern · 2026-05-24T08:21:24Z

@greptileai review

greptile-apps · 2026-05-24T08:26:25Z

Greptile Summary

This PR replaces redux-persist (last committed in 2021) with redux-remember across the NUSMods website, eliminating the persistReducer wrapper pattern and moving all persistence configuration to a single rememberEnhancer call in configureStore. A custom storage/index.ts shim transparently migrates existing persist:KEY localStorage data to the @@remember-KEY format on first read.

Core wiring: rememberReducer(combineReducers(reducers)) in reducers/index.ts replaces six individual persistReducer calls; rehydration gating moves from <PersistGate> to a purpose-built <RehydrateGate> that reads state.reduxRemember.isRehydrated via useSelector.
Migration path: migratePersistToRemember parses the old per-field-stringified format and strips _persist; remigrate is wired in as the forward migration system for future schema changes (no migrations defined yet).
Dropped migrations: The old createMigrate chains for planner (V0\u2192V1), settings (V1), and moduleBank (V1) are removed without being replayed during format conversion \u2014 very low risk given their age, but worth acknowledging.

Confidence Score: 5/5

Safe to merge as WIP; all critical rehydration paths are correctly wired and the localStorage migration shim handles format conversion without data loss for users with up-to-date stored data.

The rehydration flow is correctly gated behind isRehydrated, RehydrateGate is defined at module scope resolving earlier structural concerns, and onBeforeLift fires exactly once via useEffect. The remaining observations are structural improvements that do not cause incorrect behavior for the overwhelming majority of users.

Pay close attention to reducers/timetables.ts (the REMEMBER_REHYDRATED case is not covered by the existing stateReconciler tests) and bootstrapping/migrate-persist-to-remember.ts (old createMigrate version chains are not replayed during format conversion).

Sequence Diagram

sequenceDiagram
    participant Browser as Browser (localStorage)
    participant Storage as storage/index.ts
    participant Remember as redux-remember
    participant Reducers as rememberReducer + combineReducers
    participant App as RehydrateGate (App.tsx)

    Note over Browser,App: Page Load (first time with new code)
    Remember->>Storage: "getItem(@@remember-timetables)"
    Storage->>Browser: returns null
    Storage->>Browser: getItem(persist:timetables)
    Browser-->>Storage: JSON stringified old persist data
    Storage->>Storage: migratePersistToRemember()
    Storage-->>Remember: parsed timetables state
    Remember->>Reducers: "dispatch REMEMBER_REHYDRATED(payload=fullState)"
    Reducers->>Reducers: merge payload into state via rememberReducer
    Reducers->>Reducers: each slice reducer handles REMEMBER_REHYDRATED
    Reducers-->>App: "state.reduxRemember.isRehydrated = true"
    App->>App: useEffect fires, calls onBeforeLift()
    App->>App: render children (Router, AppShell, Routes)

    Note over Browser,App: Subsequent persists
    Remember->>Storage: "setItem(@@remember-timetables, state)"
    Storage->>Browser: stores JSON.stringify(state)
    Remember->>Reducers: dispatch REMEMBER_PERSISTED
    Reducers-->>App: "state.reduxRemember.isPersisted = true"

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
website/src/reducers/timetables.ts:31-52
**`stateReconciler` is exported and tested but not called in production**

The `stateReconciler` function is defined and exported here, and the existing tests target it directly (`describe(stateReconciler, ...)`). However, the actual reconciliation in production is performed by the duplicate inline logic in the `REMEMBER_REHYDRATED` case handler (lines 289–310), which is not covered by any test. There's also a subtle divergence: `stateReconciler` compares `inbound.academicYear === original.academicYear`, while the case handler compares `inbound.academicYear === config.academicYear`, and the new-year branch uses `defaultTimetableState` vs `original`. They're functionally equivalent today, but the dead `stateReconciler` export becomes a maintenance hazard if either code path is changed independently. Consider calling `stateReconciler` from inside the `REMEMBER_REHYDRATED` case handler so the tests actually exercise the production path.

### Issue 2 of 3
website/src/reducers/reduxRemember.ts:16-19
**`isPersisted` is tracked in state but never consumed**

`isPersisted` is set to `true` on the first `REMEMBER_PERSISTED` action and persisted in the Redux store, but no component, selector, or middleware in this PR reads it. `isRehydrated` does useful work (gating the app render in `RehydrateGate`), but `isPersisted` is currently dead state. If it isn't needed for a near-term feature, removing it would reduce store noise and avoid consumers accidentally depending on it later.

### Issue 3 of 3
website/src/bootstrapping/migrate-persist-to-remember.ts:1-22
**Old redux-persist version-based migrations are not applied during format conversion**

`migratePersistToRemember` converts the storage format (per-field JSON stringify → single JSON stringify) and strips `_persist`, but it does not re-run the old `createMigrate` version chains defined in `planner.ts` (V0→V1), `settings.ts` (V1), and `moduleBank.ts` (V1). Any user whose data was last written before those migrations ran will silently get un-migrated data into the new store. The planner case is the most concrete: V0 stored `modules` as `{ [moduleCode]: [year, semester, index] }` tuples, and the reducer now expects the V1 object shape, so accessing `module.moduleCode` on a rehydrated V0 entry would fail. The risk is low given these migrations are several years old, but it is worth documenting the assumption that no live users still hold pre-migration data.

_{Reviews (2): Last reviewed commit: "fix redux remember review issues" | Re-trigger Greptile}

greptile-apps · 2026-05-24T08:26:30Z

+    case REMEMBER_REHYDRATED: {
+      const inbound = action.payload.timetables;
+
+      if (inbound.academicYear === config.academicYear) {
+        return inbound;
+      }


Missing null guard on action.payload.timetables when no timetable data has ever been persisted (first-time user or cleared localStorage). action.payload.timetables is undefined in that case, and inbound.academicYear will throw a TypeError. The settings.ts handler correctly uses the state parameter for the same reason.

Suggested change

case REMEMBER_REHYDRATED: {

const inbound = action.payload.timetables;

if (inbound.academicYear === config.academicYear) {

return inbound;

}

case REMEMBER_REHYDRATED: {

const inbound = action.payload.timetables;

if (!inbound) return state;

if (inbound.academicYear === config.academicYear) {

return inbound;

}

Prompt To Fix With AI

This is a comment left during a code review. Path: website/src/reducers/timetables.ts Line: 289-294 Comment: Missing null guard on `action.payload.timetables` when no timetable data has ever been persisted (first-time user or cleared localStorage). `action.payload.timetables` is `undefined` in that case, and `inbound.academicYear` will throw a `TypeError`. The `settings.ts` handler correctly uses the `state` parameter for the same reason. ```suggestion case REMEMBER_REHYDRATED: { const inbound = action.payload.timetables; if (!inbound) return state; if (inbound.academicYear === config.academicYear) { return inbound; } ``` How can I resolve this? If you propose a fix, please make it concise.

I checked this against redux-remember 6.0.2 behavior and browser-tested fresh empty localStorage. The rehydrate payload is built from store.getState() first, then any loaded remembered keys are overlaid, so for a first-time user action.payload.timetables is the reducer default state rather than undefined. Fresh-storage Chromium testing also did not reproduce a crash. A defensive guard would be harmless, but the specific first-time-user failure described here does not appear to be valid.

ravern · 2026-05-24T08:36:12Z

@greptileai review

zehata · 2026-05-24T08:38:58Z

Sorry, it's been a while since I worked on this, but if I remember correctly there was some issue related to #4383 that is blocking this. If I remember correctly, there was an edge case where the rehydrate actions can contain a function in the object which causes it serialization to fail.

I should also mention, this was written awhile back when the repo first moved to using vitest, and a lint message had made me think that the repo moved to React Compiler, which is the reason why I left out the memo and callback.

ravern · 2026-05-24T09:22:26Z

Thanks, sorry for walking all over your PR - I thought it was about ready to merge + it is the end of semester. Usually for these kinds of PRs I just fix the small issues then merge.

If you prefer I can reset this to before I made any changes.

Given that this PR is blocked, you prefer to re-do this migration in another PR or leave this here till we confirm a solution in #4383?

zehata · 2026-05-24T10:00:01Z

The behavioral changes look fine actually, and I really should have changed it before but I was working on something else. I was planning to take a look through again. There were some refactoring that I was thinking might need to be done, including moving the rehydrate gate component to maybe a file together with the rest of the store logic.

Let me quickly check through it, I actually meant for this to be done before #4387 so that we can use remigrate's migration logic, so it's actually better to get this done first.

zehata · 2026-05-25T01:38:12Z

+
 const App: FC<PropsWithChildren<Props>> = ({ store }) => {
-  const onBeforeLift = () => {
+  const onBeforeLift = React.useCallback(() => {


There is nothing wrong with making this a callback but doing so is actually redundant. App never gets rerendered, I think. I added a console log and it only gets logged on initial page load.

- With changing `composeEnhancers` to `compose` in `configureStore.ts` which enables the RTK extension by default, there is no longer a need for this flag.

- I was thinking about whether we should document the function of rehydrategate to point to https://redux-remember.js.org/usage/rehydration-gate/ but I think it should be obvious enough? Let me know whether to add it

greptile-apps · 2026-05-25T03:33:27Z

PR author is not in the allowed authors list.

zehata · 2026-05-25T03:39:58Z

Given that this PR is blocked, you prefer to re-do this migration in another PR or leave this here till we confirm a solution in #4383?

@ravern Yea, let's. I don't mind resolving the merge conflicts once #4383 is fixed, do you prefer if I rebase this onto the fixed trunk? I won't delete the branch just yet, of course, do you want to close this PR now or KIV it?

Don't worry, it's not blocking me. I'm actually working on #4387 (reply in thread) so this can wait

zehata marked this pull request as draft March 31, 2026 09:34

zehata marked this pull request as ready for review March 31, 2026 12:31

zehata added 3 commits May 23, 2026 18:23

Migrate to use redux-remember instead of redux-persist

49b8083

- This was squashed from a number of commits from https://github.com/zehata/nusmods/tree/redux-remember-test

Fix stateReconciler not firing

883209b

- Previous state during rehydration is not the default state, but rather the rehydrated state itself

Rearranged enhancers to fix redux-remember actions not being logged

3aeb275

ravern force-pushed the redux-remember branch from bf55ef9 to 3aeb275 Compare May 24, 2026 08:04

Merge branch 'master' into redux-remember

8ca252b

greptile-apps Bot reviewed May 24, 2026

View reviewed changes

fix redux remember review issues

7d4f155

zehata commented May 25, 2026

View reviewed changes

zehata added 2 commits May 25, 2026 11:31

Cleanup pointless __REDUX_DEVTOOLS_EXTENSION_COMPOSE__ flag

4b43060

- With changing `composeEnhancers` to `compose` in `configureStore.ts` which enables the RTK extension by default, there is no longer a need for this flag.

Moved RehydrateGate into its own file under storage

e7fed0e

- I was thinking about whether we should document the function of rehydrategate to point to https://redux-remember.js.org/usage/rehydration-gate/ but I think it should be obvious enough? Let me know whether to add it

	// FIXME: Remove the next line when _persist is optional again.
	// Cause: https://github.com/rt2zz/redux-persist/pull/919
	// Issue: https://github.com/rt2zz/redux-persist/pull/1170
	// eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain

Conversation

zehata commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Implementation

Other Information

Uh oh!

vercel Bot commented Mar 31, 2026

Uh oh!

codecov Bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vercel Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravern commented May 24, 2026

Uh oh!

greptile-apps Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Sequence Diagram

Uh oh!

Uh oh!

greptile-apps Bot May 24, 2026

Choose a reason for hiding this comment

Uh oh!

ravern May 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ravern commented May 24, 2026

Uh oh!

zehata commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravern commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zehata commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zehata May 25, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented May 25, 2026

Uh oh!

zehata commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zehata commented Mar 31, 2026 •

edited

Loading

codecov Bot commented Mar 31, 2026 •

edited

Loading

vercel Bot commented May 24, 2026 •

edited

Loading

greptile-apps Bot commented May 24, 2026 •

edited

Loading

zehata commented May 24, 2026 •

edited

Loading

ravern commented May 24, 2026 •

edited

Loading

zehata commented May 24, 2026 •

edited

Loading

zehata commented May 25, 2026 •

edited

Loading